AutoRegressive(order=1, observed_state_names=['data_1']) + Autoregressive(order=5, observed_state_names=['data_2'])) would be a valid model with 2 observed states, but each has it's own autoregressive dynamics.

This is cool! I will review ASAP.

Note that #450 is currently blocked by what I think is a pytensor bug

I think I'm done for a first review from you on the Cycle component @jessegrabowski 🍾

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@AlexAndorra @Dekermanjian I want the names of parameters in the components to be really consistent and unsurprising. So please vote on:

1. For the sigma parameters: name_sigma vs sigma_name
2. For the initial state parameters :name_initial vs initial_name vs name
3. For assorted greek things, like rho in Cycle: name_greek vs greek_name vs descriptive_name vs name_descriptive
4. For shock state names: name vs name_shock vs name_innovation

Concrete examples for (3):
a. business_cycle_rho
b. rho_business_cycle
c. dampening_business_cycle
d. business_cycle_dampening

For (4), I'm talking about the internal state names that will end up as labels for the R and Q matrices, nothing else.

Also for default names, since all the are going to depend on the names in the multivariate case, should we:

1. Make all the default names simpler. For example LevelTrend -> level_trend, or Cycle[s={cycle}, dampen={dampen}, innovations={innovations}] -> cycle
2. Keep the complex names for univariate case, but use a simple default name when its multivariate
3. Do away with default names, and force the user to always pass a name
4. As 3, but only in the multivariate case

@AlexAndorra Also please add a test adding a cycle component to another cycle component with a different observed state name. Check the resulting matrices come out as expected.

@Dekermanjian the regression component tests are failing because of this line:

        betas = self.make_and_register_variable(f"beta_{self.name}", shape=(k_endog, k_states))

You need to drop the k_endog part of the shape if k_endog == 1, because we expect a vector in that case (don't want to give all parameters in models a dummy observed state if it doesn't matter). So something like:

        betas = self.make_and_register_variable(f"beta_{self.name}", shape=(k_endog, k_states) if k_endog > 1 else (k_states, )

Also you have beta.reshape((-1, 1)).squeeze(); this is just beta.ravel().

Finally, there are some unused computations -- you had assigned k_posdef = self.k_posdef // self.k_endog, but then didn't use it, and i guess the linter removed the unused variable but left the computation. We can just remove the whole thing.

PS: Please add some multivariate regression tests (I think you already are, but just so it's on the record somewhere, here it is), including a test where you add together two regression components with different state names

@AlexAndorra @Dekermanjian I want the names of parameters in the components to be really consistent and unsurprising. So please vote on:

1. For the sigma parameters: name_sigma vs sigma_name
2. For the initial state parameters :name_initial vs initial_name vs name
3. For assorted greek things, like rho in Cycle: name_greek vs greek_name vs descriptive_name vs name_descriptive
4. For shock state names: name vs name_shock vs name_innovation

Concrete examples for (3): a. business_cycle_rho b. rho_business_cycle c. dampening_business_cycle d. business_cycle_dampening

For (4), I'm talking about the internal state names that will end up as labels for the R and Q matrices, nothing else.

Also for default names, since all the are going to depend on the names in the multivariate case, should we:

1. Make all the default names simpler. For example LevelTrend -> level_trend, or Cycle[s={cycle}, dampen={dampen}, innovations={innovations}] -> cycle
2. Keep the complex names for univariate case, but use a simple default name when its multivariate
3. Do away with default names, and force the user to always pass a name
4. As 3, but only in the multivariate case

I personally prefer these:

1. sigma_name
2. initial_name
3. greek_name
4. innovation_name

for multivariate, I personally prefer going with option 2 or option 3 if it will also apply to the univariate case. I would prefer that univariate and multivariate are as consistent as would be possible.

@Dekermanjian I went ahead and fixed up the regression component, so please make sure to pull before you keep working.

The last steps before we merge this are to:

• Address issues with Cycle
• Add a test for forecasting with multiple observed
• Add a test for hidden state decomposition with multiple observed

I think we can also take the rough notebook that @Dekermanjian started and turn it into a tutorial about multiple observed, in the same spirit as the existing one. We can make that a separate PR if we want, but if so, we should drop the notebook from this PR.

@AlexAndorra I need your input on the naming questions I posed above, then we can make a final decision and go with it. Once that's settled, the Cycle fixes are in, and the two tests I'm asking for above are in, I'll rebase this PR so that we have one commit per component (plus one for the utilities) and merge.

Thanks for the incredibly fast progress @jessegrabowski !! Do you need a new review from me?
I'll address your Cycle comment ASAP.

Here are my votes:

1. For the sigma parameters: sigma_name
2. For the initial state parameters initial_name
3. For assorted greek things, like rho in Cycle: descriptive_name (I feel very strongly against Greek names in general, which are non-descriptive at all and increase the entry cost for users, especially since the state space modeling space uses different Greek names all over the place to designate the same quantities)
4. For shock state names: name_shock

For default names:
2. Keep the complex names for univariate case, but use a simple default name when its multivariate

Looks like we have consensus on sigma_name and initial_name. Let's agree that in all cases,the name is always at the end of everything and we have a predictable syntax of f'{something}_{name}'.

To that end, let's also go with {name}_shock for the shock dim name. I actually agree with @Dekermanjian that {name}_innovation is a better name, but I can really see myself getting sick of typing "innovation" over and over. Shock is shorter, and it's what we already use.

Regarding greek/descriptive, I'm really on the fence. I understand why @AlexAndorra is super anti-greek, and I've done implementations that specifically remove greek names (changing beta and gamma in BatchNorm to loc and scale here, for example).

On the other hand, we do pay a cost when inventing our own names, unless they are already widely known/used in the specific literature. In the loc and scale example above, ML people probably don't know what that means, so even though it's clearer to us, a practitioner coming in might not agree.

In actual fact, we already mix descriptive and greek names. In CycleComponent, we use cycle_dampening_factor and cycle_length, but then in RegressionComponent we use beta (not coef like in AutoRegressive and TimeSeasonlity :) ). And of course we use sigma everywhere, but I guess that's a "good greek", not one of those bad greeks that take our jobs.

My suggestion is to punt on this and open a new issue to address package-wide naming conventions. For now, let's just make sure everything is of the form f'{something}_{name}'.

I can really see myself getting sick of typing "innovation" over and over

Ha ha, literally the reasoning behind my choice.

On the other hand, we do pay a cost when inventing our own names, unless they are already widely known/used in the specific literature

Totally agree, it's just that the way I see it, the cost is internal: even if the descriptive names are only our own (which is not the case here, IIUC):

1. We can define them in the docstrings (in the Greek case, one has to go look on the internet and make sure it's a one-to-one correspondence
2. Descriptive is well... more descriptive -- and as Guido says, "explicit is better than implicit"

In CycleComponent, we use cycle_dampening_factor and cycle_length, but then in RegressionComponent we use beta (not coef like in AutoRegressive and TimeSeasonlity :) )

And I like CycleComponent much better for that :)

And of course we use sigma everywhere, but I guess that's a "good greek", not one of those bad greeks that take our jobs.

Masterpiece, nothing to add, you killed me 🤣
In all seriousness though, sigma is so universally associated to standard deviation that this is perfectly fine (and shorter to type)